Skip to content

Online repl checkpoint #546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
knizhnik merged 3 commits into from
Dec 18, 2024
Merged

Online repl checkpoint #546

knizhnik merged 3 commits into from
Dec 18, 2024

Conversation

@knizhnik
Copy link

@knizhnik knizhnik commented Dec 3, 2024

Checkpoint replication state not only in shutdown, but also in online checkpoint

@knizhnik knizhnik mentioned this pull request Dec 3, 2024
Merged
@tristan957
Copy link
Member

tristan957 commented Dec 6, 2024 •
edited
Loading

So @MMeent added this comment:

+/*
+ * NEON:  we use logical records to persist information of about slots, origins, relation map...
+ * If it is done inside shutdown checkpoint, then Postgres panics: "concurrent write-ahead log activity while database system is shutting down"
+ * So it before checkpoint REDO position is determined.
+ */

It sounds like the PR as written wouldn't account for this panic? Should we keep everything the same, but just move the call to CheckPointSnapBuild() after the if/else in CheckPointReplicationState()?

@tristan957
Copy link
Member

tristan957 commented Dec 6, 2024

Actually, after thinking about this more, it looks good. Let's get a review from Matthias too.

@tristan957 tristan957 requested a review from MMeent December 6, 2024 18:47
@tristan957
Copy link
Member

tristan957 commented Dec 6, 2024

A more descriptive commit message would also be useful for future git archaeologists.

@knizhnik
Copy link
Author

knizhnik commented Dec 6, 2024

So @MMeent added this comment:

+/*
+ * NEON:  we use logical records to persist information of about slots, origins, relation map...
+ * If it is done inside shutdown checkpoint, then Postgres panics: "concurrent write-ahead log activity while database system is shutting down"
+ * So it before checkpoint REDO position is determined.
+ */

It sounds like the PR as written wouldn't account for this panic? Should we keep everything the same, but just move the call to CheckPointSnapBuild() after the if/else in CheckPointReplicationState()?

I just restore behaviour which we have for all other versions of Postgres: pg14-16.
I have added this if in pg17 to minimize number of generated AUX files.
But right now I think that it is not correct: if we do not save slots info in online checkpoints then we can loose it in case of compute crash.

In any case, I think it is good idea to have the same code in all PG versions.

hlinnaka
hlinnaka reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@hlinnaka hlinnaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty confusing. Even before this PR, but this isn't helping..

  • The function is called "CheckPointReplicationState", but CheckPointRelationMap() has nothing to do with replication.
  • CheckPointRelationMap() and CheckPointReplicationOrigin() don't write any WAL records, so they don't need this special treatment. They can be called from CheckPointGuts() just like in upstream
  • In a shutdown checkpoint, all these functions are now being called twice, once in the PreCheckPointGuts() stage and again in CheckPointGuts(). Seems unnecessary, but also wrong; won't those functions PANIC again, when the try to write the WAL records?

@knizhnik
Copy link
Author

knizhnik commented Dec 10, 2024

Yes, CheckPointRelationMap() has not relation to replication.
But it is called before CheckPointReplicationSlots and CheckPointLogicalRewriteHeap in CheclpointGuts this his why I thought that later ones can somehow depends on it and it is better to reserve order.

CheckPointReplicationOrigin() writes "pg_logical/replorigin_checkpoint" file. But we do not persist it using AUX mechanism. Actually I don't remember why. May be just because it is not needed because origins are wallowed in any case.

It is real bug that CheckPointReplicationState is called twice - I forget to copy correspondent check (which is done in pg 14/15/16). I will fix it.

@knizhnik
Copy link
Author

knizhnik commented Dec 10, 2024

I committed fix of double call of CheckPointReplicationState.
So now pg17 is ding exactly the same as all there versions.

Do you think that we should include in this PR all other changes: move CheckPointReplicationOrigin() and CheckPointRelationMap() out of CheckPointReplicationState?

I agree with you that they are not writing WAL and so there is no need to call them here.
But as I wrote above, I prefer to reserve order of calling this functions in CheckpointGuts and CheckPointReplicationOrigin() definitely is related to replication state, although in not walloging written file.

I prefer to leave it is as it is now or create separate PR for it, because it should affect all other Postgres versions.

@knizhnik knizhnik force-pushed the online_repl_checkpoint branch from 9845a53 to 4671da9 Compare December 11, 2024 12:14
@knizhnik knizhnik requested a review from hlinnaka December 12, 2024 13:59
@knizhnik knizhnik force-pushed the online_repl_checkpoint branch from 4671da9 to 7e3f397 Compare December 16, 2024 06:33
@hlinnaka
Copy link
Contributor

hlinnaka commented Dec 17, 2024

CheckPointReplicationOrigin() writes "pg_logical/replorigin_checkpoint" file. But we do not persist it using AUX mechanism. Actually I don't remember why. May be just because it is not needed because origins are wallowed in any case.

Related: neondatabase/neon#8620

hlinnaka
hlinnaka approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@hlinnaka hlinnaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I committed fix of double call of CheckPointReplicationState. So now pg17 is ding exactly the same as all there versions.

Do you think that we should include in this PR all other changes: move CheckPointReplicationOrigin() and CheckPointRelationMap() out of CheckPointReplicationState?

I agree with you that they are not writing WAL and so there is no need to call them here. But as I wrote above, I prefer to reserve order of calling this functions in CheckpointGuts and CheckPointReplicationOrigin() definitely is related to replication state, although in not walloging written file.

I prefer to leave it is as it is now or create separate PR for it, because it should affect all other Postgres versions.

Ok, let's commit this, to bring all the Postgres versions to the same state, and open a separate PR for the other cleanup.

github-merge-queue bot pushed a commit to neondatabase/neon that referenced this pull request Dec 18, 2024
@knizhnik
Online checkpoint replication state (#9976)
aaf980f 
## Problem

See https://neondb.slack.com/archives/C04DGM6SMTM/p1733180965970089

Replication state is checkpointed only by shutdown checkpoint.
It means that replication snapshots are not removed till compute
shutdown.

## Summary of changes

Checkpoint replication state during online checkpoint

Related Postgres PR:
neondatabase/postgres#546

Co-authored-by: Konstantin Knizhnik <knizhnik@neon.tech>
@knizhnik knizhnik merged commit 7e3f397 into REL_17_STABLE_neon Dec 18, 2024
1 check passed
@knizhnik knizhnik deleted the online_repl_checkpoint branch December 18, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hlinnaka hlinnaka hlinnaka approved these changes

@tristan957 tristan957 tristan957 approved these changes

@MMeent MMeent Awaiting requested review from MMeent

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@knizhnik @tristan957 @hlinnaka